Coding standard : Opening brace on separate line. Also, recommend using a name like xrdp_sec_process_monitor_stream so it's easy to find the function.

Fixed after merged with astyle.

Coding standard : Use '_' in preference to camel-case for new variables

Fixed.

I appreciate that these values aren't given a name in MS-RDBBCGR, but is it worth adding defines for them for the scope of this file to improve readability and add a reference (e.g.):-

/* Desktop limits [MS-RDPBCGR] 2.2.1.3.6 */
#define MIN_VIRTUAL_DESKTOP_WIDTH 200
(etc)

Updated by adding these to ms-rdpbcgr.h

Works great.

Needs test for NULL return here if an invalid PDU is received.

Good catch. Fixed.

I don't understand the severity change of this message. An error seems appropriate here. Am I missing something?

I think this was leftover from my debugging while developing the change. Reverted.

Coding standard again, also a few lines below.

Fixed with merge to astyle.

This can return NULL, which will break process_dynamic_monitor_description(). Suggest adding a guard here, or in that function.

Added null check to process_dynamic_monitor_description.

I'm very much against using goto in new code, since it is prone to misuse. Even though it's usage here seems reasonable, I'd still prefer replacing it with a structured programing construct.

One option for removing the goto's is to allocate the description struct in the xrdp_sec_process_mcs_data_monitors function, since you already unconditionally free the description struct there currently.

This way of exiting a method is actually what a lot of C programmers at Microsoft use (I used to work there), and other than exiting nested loops this is the only legitimate use of goto I'm aware of.

Still, I hear you. I've refactored it so that process_monitor_stream accepts a display_size_description structure that needs to be pre-allocated by the calling functions, and since there are only two places where it's used/allocated/destroyed unconditionally, this shouldn't be a problem.

I find it very useful to have the spec and struct refence in the error message when there is a parsing failure. Can you please add the spec and struct reference in the error message that matches the struct that is being parsed.

Those values aren't directly specified in the Microsoft specification, they're derived. However, I've made an attempt to clarify where they come from and make the error messages clearer.

why are these two log messages at different levels and using different formats?
can you please make them consistent, and include the spec and struct reference in the messages.

Updated.

This trace log message seems to have been removed. Can you please add it back in the xrdp_sec_process_mcs_data_monitors function?

The flow of the logic has changed with this PR; making it difficult to put this somewhere elegant, but I've done my best to add it back

this seem like it warrants an error message (as well as all of the other error cases in this function). The rest of the code use the error log level for these situations.

I think I fixed this?

If we get here it should indicate a bug in the code. I think this should be logged as an error.
Also, @matt335672 suggested that we log these error conditions using LOG instead of LOG_DEVEL so that we see this error message in big reports from users.

Yes - I appreciate that a 'null pointer' error may not mean much to a user, but if it's not easy or possible to find a user-focussed answer it's better for them to have something to report than nothing at all.

Also, I agree this should be LOG_LEVEL_ERROR

Another also - please split the string into multiple string literals "..." "..." and reformat to 80 chars (or a little bit over) max line width as per the coding standard. There are quite a few other instances of this further below too. This bit of shell magic picks out the culprits:-

grep -n '' libxrdp/xrdp_sec.c | sed -ne '2307,2505p' | grep -E ':.{80}'

I appreciate that other bits of this file are also breaking the standard here, but for new code I think we should try to avoid this where possible.

FYI You can remove the function name orchid from the log message since they are added automatically when using a debug build.

But this isn't a debug log (DEVEL) -- If it's not a debug build, does the orchid still appear?

Good point - the orchid won't appear in a non-debug build.

Since the RDP spec has many extension specs, can you please include the spec name abbreviation in the message. Eg. [MS-RDPBCGR]

I'm curious as to why pointer arithmetic is used here instead of array indexing?

I'm happy to do it either way, I liked the way pointer arithmetic worked better here, but if you'd prefer indexing I'll change it.

Can you clarify what the difference between what these two fields represent?
it seems like minfo is the client side representation of the monitor layout, and minfo_wm is the server side representation of the same monitor layout. Is that correct?

Pardon me for jumping in, but since I'm here and I've used these values I can say that that's essentially correct The _wm values are normalised for a top-left co-ordinate of (0,0), which is useful for some bits of the software, namely the VNC multimon stuff and the login window.

Exactly. Does this need further documentation in the code?

Personally I'm happy with the existing comment

Where do these initial rectangle bounds come from?
it looks like these initial value get overwritten on line 2481, am I missing something that makes these initial values important?

Also, the variable name rect isn't very descriptive, and I want to make sure I understand what's it's calculating correctly: rect's field values are in coordinate space, and rect represents the minimum sized bounding box which encloses all monitors. Is this correct?

Updated rect to all_monitors_encompassing_bounds -- You're right the initial values are from an older version of the code, so I've removed them.

Since this log message is for sys admins can you please add more context and expand or avoid abbreviations.

Updated.

Can you please use string formatting with the constant CLIENT_MONITOR_DATA_MAXIMUM_MONITORS instead of hard coding the value in the error message so that the error message doesn't get out-of-sync withthe constant value.

Good catch. Updated.

monitor_layout->is_primary isn't assigned anywhere in the new version, but was assigned in the old version. Is this intentional?

You're right. Fixed.

why do we only set the session's width and height when rect is well-formed? it seems like if rect is not well-formed then xrdp should log and return an error.

Also, why was the +1 from (x2 - x1) + 1 removed?

Good point -- I added the error.

The +1 was removed because it wasn't present in the similar code that came from the resizing code, but it probably should be there as it's actually more correct.

The description->session_width and description->session_height are not initialized by this function, and only conditionally set if the rect variable is well-formed. This means that this function could be logging values in the error message that were not sent by the client. Can you please fix this so that the error message always shows what was received from the client.

I think I addressed this? Let me know.

The width and height fields are redundant with the right and bottom fields. To avoid having out-of-sync fields, I think we should only keep the right and bottom fields, and calculate width and height when needed.

Also, all of the DISPLAYCONTROL_MONITOR_LAYOUT are optional, but there is no way to determine of the values in those fields are valid or not. Can we make them always valid, and just put in some sensible default values?

I don't see where in https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpedisp/ea2de591-9203-42cd-9908-be7a55237d1c the values in the structure are optional...

I was trying to keep the data in the monitor info structure to be representative of the data sent from the client, but I agree data consistency is more important. Updated.

Sorry my comment was not clear. You are correct that all of the fields in the spec are mandatory.

What I meant by optional fields is that with the changes to the xrdp monitor_info structure, now the physical size and scaling fields are are not populated when parsing a TS_MONITOR_DEF.

I suggest that when the code parses a TS_MONITOR_DEF then all of the monitor_info field are populated with reasonable defaults, so that we don't have any undefined data in those fields under some conditions.

Per spec it must be 40.

Which spec? when I find this message in the log file, where will I find where in the spec this value is defined?
I recommend moving the code comment into the error message, so that the error message has the full context of the problem.

Updated.

Yes - I appreciate that a 'null pointer' error may not mean much to a user, but if it's not easy or possible to find a user-focussed answer it's better for them to have something to report than nothing at all.

Pardon me for jumping in, but since I'm here and I've used these values I can say that that's essentially correct The _wm values are normalised for a top-left co-ordinate of (0,0), which is useful for some bits of the software, namely the VNC multimon stuff and the login window.

There's some duplication here - the new struct display_size_description contains fields which are already present in this file further down.

I know. I didn't want to change xrdp_client_info since that will incur a revision of the header file and also affect xorgxrdp. I want to make the change to unify these two, but not in this PR... I want to do it for a PR that's focused on unifying these two data structures and leave the ones that change xorgxrdp more focused and linked. Make sense?

Yes that's fine- you can raise a PR now and mark it as draft if you like to keep the book-keeping simple.

I decided to actually just do it now. It's not as terrible as I thought.

See comments for xrdp_sec.c - delete this function declaration.

Updated.

To add to @aquesnel's comment - there's another more significant issue here. This function is defined and used here, but also used in xrdp_mm.c. At the moment there isn't a common declaration for this function in an include file, and so the compiler isn't able to check that the function signature is the same between xrdp_mm.c and this file. So if you added an extra parameter to this function and forgot to update xrdp_mm.c we'd end up with a difficult-to-debug situation.

The function is defined in libxrdp, and as far as I can tell, the naming convention here is to prefix global functions from this library with libxrdp_ and declare them in libxrdp/libxrdpinc.h. I don't think this is written down anywhere, and I've only just worked it out myself, but if you look at the calls that xrdp makes to libxrdp, you'll see what I mean.

So I think this function should be called libxrdp_process_monitor_stream()

Also, we're trying for new stuff to add proper function documentation headers using Doxygen-style comments. At some stage we can then start generating development docs from the code using Doxygen like FreeRDP.

My suggestion here is:-

• Rename this function as libxrdp_process_monitor_stream()
• Add a declaration for the function to libxrdp/libxrdpinc.h
• Add function documentation to the declaration using a Doxygen-style header. Examples are in common/log.h or common/string_calls.h. Both of these are fairly new.

There's the small matter of struct display_size_description which isn't specified in libxrdp/libxrdpinc.h. One solution to this would be to move the whole struct to libxrdp/libxrdpinc.h and call it struct xrdp_display_size_description. Personally I don't think this is necessary. Since you're just using a pointer to the structure in your function prototype you can use an incomplete type declaration for the struct in libxrdp/libxrdpinc.h like this:-

/* Defined in xrdp_client_info.h */
struct display_size_description;

<stuff snipped>

/**
 * Doxygen function comment
 * @param s . . . 
 * @param description . . .
 * @param full_parameters . . .
 * @return . . .
 */
int
libxrdp_process_monitor_stream(struct stream *s,
                               struct display_size_description *description,
                               int full_parameters);

Let me know if that doesn't makes sense.

Makes sense! Updated.